-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement FormData.get and FormData.getAll #388
base: master
Are you sure you want to change the base?
Conversation
4fde543
to
2077a63
Compare
Th appveyor test is failing randomly I don't know why (I tried to amend and push another time, and it failed on another test case) |
@alexindigo could you take a look to this mate. I need this feature to be able to publish a package and would be great if we could have it. It seems that the Appveyor is failing because of permissions issues, but the PR is fine. |
2077a63
to
7e3f709
Compare
@@ -31,6 +31,7 @@ function FormData(options) { | |||
this._overheadLength = 0; | |||
this._valueLength = 0; | |||
this._valuesToMeasure = []; | |||
this._entryList = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused with this.
You use an array as the data structure but then you use it as an object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad !
It works because Javascript Array are objects, and thus it become attribute of an object, I will change this to a map ;)
7e3f709
to
3d52f6c
Compare
@jdeniau actually I would use a list for this and create a list of objects. I know that from data structure makes sense but following the spec I believe that From documentation
So maybe is better to have a list of objects
I am trying to finish the spec and probably maybe would be better if you change the current implementation So far, function createEntry(name, value, filename) {
// TODO: handle Blob and File
// Related to https://xhr.spec.whatwg.org/#concept-formdata-entry
// Case 1:
// If value is a Blob object and not a File object, then set value to a new
// File object, representing the same bytes, whose name attribute value is
// "blob".
// Case 2:
// If value is (now) a File object and filename is given, then set value to a
// new File object, representing the same bytes, whose name attribute value
// is filename.
var entry = {
name: name,
value: value,
};
return entry;
} I created this functions because it seems that we will need to do some complex stuff with the entries https://xhr.spec.whatwg.org/#concept-formdata-entry So basically for now you could do The filename come from the API, on
So one expects just a value and the other one expects a File/Blob I guess. |
lib/form_data.js
Outdated
@@ -31,6 +31,7 @@ function FormData(options) { | |||
this._overheadLength = 0; | |||
this._valueLength = 0; | |||
this._valuesToMeasure = []; | |||
this._entryList = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this._entryList = {}; | |
this._entryList = []; |
We need to use a list rather than an array. The current spec allow you to append to the list multiple values for the same key
lib/form_data.js
Outdated
if (!this._entryList[field]) { | ||
this._entryList[field] = []; | ||
} | ||
this._entryList[field].push(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Append an object with the following shape
{ name: '', value: '' }
append
do not remove previous entry, just do an append
on the list.
this._entryList[field].push(value); | |
// move this function out of this scope, this is just so you have an idea about it. | |
function createEntry(name, value, filename) { | |
// TODO: handle Blob and File | |
// Related to https://xhr.spec.whatwg.org/#concept-formdata-entry | |
// Case 1: | |
// If value is a Blob object and not a File object, then set value to a new | |
// File object, representing the same bytes, whose name attribute value is | |
// "blob". | |
// Case 2: | |
// If value is (now) a File object and filename is given, then set value to a | |
// new File object, representing the same bytes, whose name attribute value | |
// is filename. | |
var entry = { | |
name: name, | |
value: value, | |
}; | |
return entry; | |
} | |
const entry = createEntry(field, value, filename); | |
this._entryList.push(entry); |
lib/form_data.js
Outdated
}; | ||
|
||
FormData.prototype.get = function(field) { | ||
return this._entryList[field] ? this._entryList[field][0] : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the spec
The get(name) method, when invoked, must return the value of the first entry whose name is
name
from the context object’s entry list, and null otherwise.
Do an Array.find
return this._entryList[field] ? this._entryList[field][0] : null; | |
var foundEntry = this._entryList.find(function(entry) { | |
return entry.name === field; | |
}); | |
return foundEntry === undefined ? null: foundEntry.value |
lib/form_data.js
Outdated
}; | ||
|
||
FormData.prototype.getAll = function(field) { | ||
return this._entryList[field] || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the spec
The getAll(name) method, when invoked, must return the values of all entries whose name is
name
, in order, from the context object’s entry list, and the empty list otherwise.
Do an Array.filter
in the list.
return this._entryList[field] || []; | |
return this._entryList.reduce(function (acc, entry) { | |
if(entry.name === field) { | |
acc.push(entry.value); | |
} | |
return acc; | |
}, []); | |
Hey @jdeniau, for me to be able to finish the other APIs I will need the changes that I suggest. Would be better for me to work on top what you did already rather than duplicating the work. |
Looks good to me, I will work on this, but I did not quite well understand the [constructing the entry list(https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#constructing-the-form-data-set) part 🤔 |
@jdeniau well that one is for the web actually.
If you pass a form you need to loop throw the list of inputs under that form and do an But this code is only for NodeJS unless you are trying to monkey patch that as well. |
fa9dbea
to
48d93c2
Compare
The implementation is maybe very simplistic but it may do the job
See: